Warn before Sparkle update relaunch kills terminal sessions#1986
Open
austinywang wants to merge 4 commits intomainfrom
Open
Warn before Sparkle update relaunch kills terminal sessions#1986austinywang wants to merge 4 commits intomainfrom
austinywang wants to merge 4 commits intomainfrom
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughAdds a user confirmation step before relaunching the app for updates when active terminal sessions exist, introduces two localized warning strings, a new AppDelegate API to summarize affected terminals and present an NSAlert, and wires the confirmation into the update driver to gate relaunch/install actions. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant UpdateDriver
participant AppDelegate
participant NSAlertUI as NSAlert UI
UpdateDriver->>AppDelegate: invoke confirmUpdateRelaunchIfNeededHandler()
AppDelegate->>AppDelegate: updateRelaunchTerminalSummary()
AppDelegate->>NSAlertUI: present localized warning (title + message with counts)
NSAlertUI->>User: show dialog ("Restart Now" / "Later")
User-->>NSAlertUI: choose button
NSAlertUI-->>AppDelegate: return choice
alt User chose "Restart Now"
AppDelegate-->>UpdateDriver: true
UpdateDriver->>UpdateDriver: proceed with install/relaunch action
else User chose "Later"
AppDelegate-->>UpdateDriver: false
UpdateDriver->>UpdateDriver: log deferred relaunch, skip action
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
⚔️ Resolve merge conflicts
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Contributor
Greptile SummaryThis PR adds an incremental safety guard against accidental terminal session loss during Sparkle auto-updates. Before cmux terminates to install an update, it now enumerates active Key changes:
Minor findings (P2):
Confidence Score: 5/5
Important Files Changed
Sequence DiagramsequenceDiagram
participant Sparkle
participant UpdateDriver
participant AppDelegate
participant NSAlert
Sparkle->>UpdateDriver: showInstallingUpdate(applicationTerminated: false, retryTerminatingApplication)
UpdateDriver->>UpdateDriver: makeGuardedUpdateRelaunchAction(applicationTerminated, action)
UpdateDriver->>Sparkle: passes guardedRetryTerminatingApplication back
alt User triggers relaunch (applicationTerminated == false)
Sparkle->>UpdateDriver: guardedRetryTerminatingApplication()
UpdateDriver->>UpdateDriver: Task @MainActor
UpdateDriver->>AppDelegate: confirmUpdateRelaunchIfNeeded()
AppDelegate->>AppDelegate: updateRelaunchTerminalSummary()
alt No active terminal sessions
AppDelegate-->>UpdateDriver: return true
UpdateDriver->>Sparkle: action()
else Active terminal sessions found
AppDelegate->>NSAlert: runModal()
alt User clicks Restart Now
NSAlert-->>AppDelegate: alertFirstButtonReturn
AppDelegate-->>UpdateDriver: return true
UpdateDriver->>Sparkle: action()
else User clicks Later
NSAlert-->>AppDelegate: alertSecondButtonReturn
AppDelegate-->>UpdateDriver: return false
Note over UpdateDriver: action() not called — Sparkle waits
end
end
else applicationTerminated == true
Sparkle->>UpdateDriver: guardedRetryTerminatingApplication()
UpdateDriver->>UpdateDriver: Task @MainActor
Note over UpdateDriver: bypass warning
UpdateDriver->>Sparkle: action() immediately
end
Reviews (1): Last reviewed commit: "Warn before update relaunch kills termin..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+471
to
+486
| { [weak self] in | ||
| Task { @MainActor [weak self] in | ||
| guard self != nil else { return } | ||
| if applicationTerminated { | ||
| action() | ||
| return | ||
| } | ||
|
|
||
| guard AppDelegate.shared?.confirmUpdateRelaunchIfNeeded() ?? true else { | ||
| UpdateLogStore.shared.append("update relaunch deferred due to active terminal sessions") | ||
| return | ||
| } | ||
|
|
||
| action() | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
[weak self] capture
The outer closure captures [weak self] but never references self directly — only the inner Task closure uses it (and re-captures [weak self] independently). The outer capture has no effect and creates a misleading impression that self is used in the outer scope.
Suggested change
| { [weak self] in | |
| Task { @MainActor [weak self] in | |
| guard self != nil else { return } | |
| if applicationTerminated { | |
| action() | |
| return | |
| } | |
| guard AppDelegate.shared?.confirmUpdateRelaunchIfNeeded() ?? true else { | |
| UpdateLogStore.shared.append("update relaunch deferred due to active terminal sessions") | |
| return | |
| } | |
| action() | |
| } | |
| } | |
| { in | |
| Task { @MainActor [weak self] in | |
| guard self != nil else { return } | |
| if applicationTerminated { | |
| action() | |
| return | |
| } | |
| guard AppDelegate.shared?.confirmUpdateRelaunchIfNeeded() ?? true else { | |
| UpdateLogStore.shared.append("update relaunch deferred due to active terminal sessions") | |
| return | |
| } | |
| action() | |
| } | |
| } |
| updateController.attemptUpdate() | ||
| } | ||
|
|
||
| func confirmUpdateRelaunchIfNeeded() -> Bool { |
Contributor
There was a problem hiding this comment.
@MainActor annotation
confirmUpdateRelaunchIfNeeded() calls NSAlert.runModal() (which must run on the main thread) and reads main-thread-only state via updateRelaunchTerminalSummary() (mainWindowContexts, tabManager). The current call site wraps it in Task { @MainActor } correctly, but without the annotation the compiler won't prevent future callers from invoking this method off-actor.
Suggested change
| func confirmUpdateRelaunchIfNeeded() -> Bool { | |
| @MainActor | |
| func confirmUpdateRelaunchIfNeeded() -> Bool { |
Comment on lines
+6043
to
+6050
|
|
||
| if workspace.panelNeedsConfirmClose( | ||
| panelId: panelId, | ||
| fallbackNeedsConfirmClose: terminalPanel.needsConfirmClose() | ||
| ) { | ||
| runningCommandCount += 1 | ||
| } | ||
|
|
Contributor
There was a problem hiding this comment.
runningCommandCount counts panels, not commands
The counter increments once per TerminalPanel whose panelNeedsConfirmClose returns true, but the UI label reads "Running commands detected: N" — implying N individual commands. If a single terminal panel hosts a pipeline of several processes, the number displayed will be lower than the actual running-command count, which could cause users to underestimate the impact of the restart.
Consider renaming the field and label to "Terminal sessions with running commands:" to match what the code actually measures, or documenting the known limitation in a comment.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
Resources/Localizable.xcstrings (1)
77801-77817: Consider adding pluralization for better grammar (optional refinement).The message currently uses fixed plural forms ("Workspaces affected: %1$lld") which will read slightly awkwardly when counts are 1. As per coding guidelines, cmux prefers the
.one/.otherkey pattern for pluralized strings (e.g.,update.relaunchWarning.workspaces.oneandupdate.relaunchWarning.workspaces.other).However, for a statistics-style warning dialog showing multiple counts, the current implementation is clear and functional. This refinement can be deferred if you prefer to keep the warning message simpler.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Resources/Localizable.xcstrings` around lines 77801 - 77817, The string key update.relaunchWarning.message uses fixed-count phrases which can be improved by introducing pluralized keys (e.g., update.relaunchWarning.workspaces.one / .other, update.relaunchWarning.terminals.one / .other, update.relaunchWarning.runningCommands.one / .other, update.relaunchWarning.remoteTerminals.one / .other) and then composing the final dialog by selecting the correct plural form for each count; update the Localizable.xcstrings JSON to add these .one/.other entries (with English and Japanese localizations) and change the dialog construction code to format the message by inserting the appropriate pluralized fragments instead of the single update.relaunchWarning.message block so the text reads correctly for singular vs plural counts.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Sources/AppDelegate.swift`:
- Around line 6016-6028: The function updateRelaunchTerminalSummary currently
only appends the active tabManager when managers.isEmpty, which can omit the
active manager if mainWindowContexts is temporarily out of sync; change the
collection logic in updateRelaunchTerminalSummary to always include both sources
(iterate mainWindowContexts and also include the optional tabManager) and then
dedupe by identity (use ObjectIdentifier like the existing seenManagers set) so
every unique TabManager is visited exactly once; reference the symbols
mainWindowContexts, tabManager, seenManagers, and the
updateRelaunchTerminalSummary function and mirror the “visit both sources, then
dedupe by identity” pattern used by forEachTerminalPanel.
- Around line 6044-6048: The code is incrementing runningCommandCount for panels
where workspace.panelNeedsConfirmClose(panelId:fallbackNeedsConfirmClose:)
(which uses panelShellActivityStates in Workspace.swift) is true — this
represents sessions requiring close confirmation, not actual running commands;
change the metric/variable and user-facing text accordingly (e.g., rename
runningCommandCount to sessionsRequiringCloseConfirmationCount and update any
log/message like "Running commands detected" to "Sessions requiring close
confirmation") or, if you need true command counts, replace the increment with
logic that queries the real command enumeration source instead of
terminalPanel.needsConfirmClose() / panelShellActivityStates.
- Around line 5912-5923: The alert.informativeText is using a single localized
paragraph ("update.relaunchWarning.message") with four embedded counts
(summary.workspaceCount, summary.terminalSessionCount,
summary.runningCommandCount, summary.remoteTerminalSessionCount) which prevents
proper plural handling; change to use separate plural-aware localization keys
(e.g. "update.relaunchWarning.workspaces", ".terminalSessions",
".runningCommands", ".remoteTerminalSessions") each with .one/.other forms, then
build alert.informativeText by composing these four localized plural strings
plus the static header lines so each count can be pluralized independently;
update the call-site that constructs alert.informativeText to call
String(localized: "update.relaunchWarning.workspaces", ... ,
summary.workspaceCount) etc., and remove the single combined
"update.relaunchWarning.message" usage.
---
Nitpick comments:
In `@Resources/Localizable.xcstrings`:
- Around line 77801-77817: The string key update.relaunchWarning.message uses
fixed-count phrases which can be improved by introducing pluralized keys (e.g.,
update.relaunchWarning.workspaces.one / .other,
update.relaunchWarning.terminals.one / .other,
update.relaunchWarning.runningCommands.one / .other,
update.relaunchWarning.remoteTerminals.one / .other) and then composing the
final dialog by selecting the correct plural form for each count; update the
Localizable.xcstrings JSON to add these .one/.other entries (with English and
Japanese localizations) and change the dialog construction code to format the
message by inserting the appropriate pluralized fragments instead of the single
update.relaunchWarning.message block so the text reads correctly for singular vs
plural counts.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d94cbce3-217d-458f-b1c3-50aa2ce5a8e4
📒 Files selected for processing (3)
Resources/Localizable.xcstringsSources/AppDelegate.swiftSources/Update/UpdateDriver.swift
Comment on lines
+5912
to
+5923
| alert.informativeText = String( | ||
| localized: "update.relaunchWarning.message", | ||
| defaultValue: """ | ||
| This update will relaunch cmux and terminate in-app terminal sessions. | ||
|
|
||
| Workspaces affected: \(summary.workspaceCount) | ||
| Terminal sessions affected: \(summary.terminalSessionCount) | ||
| Running commands detected: \(summary.runningCommandCount) | ||
| Remote / SSH terminal sessions: \(summary.remoteTerminalSessionCount) | ||
|
|
||
| Shells, SSH connections, and agents attached to these PTYs will be terminated. | ||
| """ |
There was a problem hiding this comment.
Use plural-aware localized strings for the count lines.
This packs four counted labels into one fixed-plural paragraph, so translators cannot independently handle 1 vs n for each line. Split these into separate .one / .other resources (or otherwise plural-aware localized lines) before composing the alert body.
Based on learnings: In Swift files (cmux project), when handling pluralized strings, prefer using localization keys with the ICU-style plural forms .one and .other.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Sources/AppDelegate.swift` around lines 5912 - 5923, The
alert.informativeText is using a single localized paragraph
("update.relaunchWarning.message") with four embedded counts
(summary.workspaceCount, summary.terminalSessionCount,
summary.runningCommandCount, summary.remoteTerminalSessionCount) which prevents
proper plural handling; change to use separate plural-aware localization keys
(e.g. "update.relaunchWarning.workspaces", ".terminalSessions",
".runningCommands", ".remoteTerminalSessions") each with .one/.other forms, then
build alert.informativeText by composing these four localized plural strings
plus the static header lines so each count can be pluralized independently;
update the call-site that constructs alert.informativeText to call
String(localized: "update.relaunchWarning.workspaces", ... ,
summary.workspaceCount) etc., and remove the single combined
"update.relaunchWarning.message" usage.
Comment on lines
+6016
to
+6028
| private func updateRelaunchTerminalSummary() -> UpdateRelaunchTerminalSummary? { | ||
| var seenManagers = Set<ObjectIdentifier>() | ||
| var managers: [TabManager] = [] | ||
|
|
||
| for context in mainWindowContexts.values { | ||
| let managerId = ObjectIdentifier(context.tabManager) | ||
| guard seenManagers.insert(managerId).inserted else { continue } | ||
| managers.append(context.tabManager) | ||
| } | ||
|
|
||
| if managers.isEmpty, let tabManager { | ||
| managers.append(tabManager) | ||
| } |
There was a problem hiding this comment.
Do not hide the active tabManager behind an empty-array fallback.
This only appends tabManager when mainWindowContexts is empty. If window/context bookkeeping is temporarily out of sync, the active manager's terminals can be omitted from the summary and the relaunch warning can undercount or disappear. The existing forEachTerminalPanel helper later in this file already uses the safer “visit both sources, then dedupe by identity” pattern.
♻️ Suggested adjustment
for context in mainWindowContexts.values {
let managerId = ObjectIdentifier(context.tabManager)
guard seenManagers.insert(managerId).inserted else { continue }
managers.append(context.tabManager)
}
- if managers.isEmpty, let tabManager {
- managers.append(tabManager)
+ if let tabManager {
+ let managerId = ObjectIdentifier(tabManager)
+ if seenManagers.insert(managerId).inserted {
+ managers.append(tabManager)
+ }
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| private func updateRelaunchTerminalSummary() -> UpdateRelaunchTerminalSummary? { | |
| var seenManagers = Set<ObjectIdentifier>() | |
| var managers: [TabManager] = [] | |
| for context in mainWindowContexts.values { | |
| let managerId = ObjectIdentifier(context.tabManager) | |
| guard seenManagers.insert(managerId).inserted else { continue } | |
| managers.append(context.tabManager) | |
| } | |
| if managers.isEmpty, let tabManager { | |
| managers.append(tabManager) | |
| } | |
| private func updateRelaunchTerminalSummary() -> UpdateRelaunchTerminalSummary? { | |
| var seenManagers = Set<ObjectIdentifier>() | |
| var managers: [TabManager] = [] | |
| for context in mainWindowContexts.values { | |
| let managerId = ObjectIdentifier(context.tabManager) | |
| guard seenManagers.insert(managerId).inserted else { continue } | |
| managers.append(context.tabManager) | |
| } | |
| if let tabManager { | |
| let managerId = ObjectIdentifier(tabManager) | |
| if seenManagers.insert(managerId).inserted { | |
| managers.append(tabManager) | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Sources/AppDelegate.swift` around lines 6016 - 6028, The function
updateRelaunchTerminalSummary currently only appends the active tabManager when
managers.isEmpty, which can omit the active manager if mainWindowContexts is
temporarily out of sync; change the collection logic in
updateRelaunchTerminalSummary to always include both sources (iterate
mainWindowContexts and also include the optional tabManager) and then dedupe by
identity (use ObjectIdentifier like the existing seenManagers set) so every
unique TabManager is visited exactly once; reference the symbols
mainWindowContexts, tabManager, seenManagers, and the
updateRelaunchTerminalSummary function and mirror the “visit both sources, then
dedupe by identity” pattern used by forEachTerminalPanel.
Comment on lines
+6044
to
+6048
| if workspace.panelNeedsConfirmClose( | ||
| panelId: panelId, | ||
| fallbackNeedsConfirmClose: terminalPanel.needsConfirmClose() | ||
| ) { | ||
| runningCommandCount += 1 |
There was a problem hiding this comment.
This is a busy-session count, not a command count.
This increments once per terminal panel based on panelNeedsConfirmClose(...); Sources/Workspace.swift:6074-6079 backs that with panelShellActivityStates, so the downstream “Running commands detected” wording reads more precise than the data you actually have. Rename the metric/message to something like “sessions requiring close confirmation,” or switch to a real command enumeration source.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Sources/AppDelegate.swift` around lines 6044 - 6048, The code is incrementing
runningCommandCount for panels where
workspace.panelNeedsConfirmClose(panelId:fallbackNeedsConfirmClose:) (which uses
panelShellActivityStates in Workspace.swift) is true — this represents sessions
requiring close confirmation, not actual running commands; change the
metric/variable and user-facing text accordingly (e.g., rename
runningCommandCount to sessionsRequiringCloseConfirmationCount and update any
log/message like "Running commands detected" to "Sessions requiring close
confirmation") or, if you need true command counts, replace the increment with
logic that queries the real command enumeration source instead of
terminalPanel.needsConfirmClose() / panelShellActivityStates.
There was a problem hiding this comment.
♻️ Duplicate comments (2)
Sources/AppDelegate.swift (2)
6025-6033:⚠️ Potential issue | 🟠 MajorAlways include the current
tabManagerin this scan.Line 6031 only appends
tabManagerwhenmainWindowContextsis empty. If that registry is momentarily stale,terminalSessionCountcan fall to 0 and this warning path gets skipped even though live terminals still exist. MirrorforEachTerminalPanel(_:)and visit both sources, then dedupe byObjectIdentifier.♻️ Suggested adjustment
for context in mainWindowContexts.values { let managerId = ObjectIdentifier(context.tabManager) guard seenManagers.insert(managerId).inserted else { continue } managers.append(context.tabManager) } - if managers.isEmpty, let tabManager { - managers.append(tabManager) + if let tabManager { + let managerId = ObjectIdentifier(tabManager) + if seenManagers.insert(managerId).inserted { + managers.append(tabManager) + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/AppDelegate.swift` around lines 6025 - 6033, The scan over window contexts currently only appends the current tabManager when no managers were found, which can miss live terminals if mainWindowContexts is stale; modify the logic that builds managers (the loop over mainWindowContexts and the conditional that appends tabManager) so you always visit both sources—iterate mainWindowContexts.values and also visit the current tabManager (if non-nil) and use ObjectIdentifier + seenManagers.insert(...) to dedupe exactly as is done for the contexts (mirror forEachTerminalPanel(_:)'s approach), ensuring tabManager is added when not already seen rather than only when managers.isEmpty.
5917-5929:⚠️ Potential issue | 🟡 MinorSplit these counts into plural-aware localized lines.
Line 5917 folds four counted labels into one localized paragraph, so each line cannot get its own singular/plural form. Use separate
.one/.otherkeys for the counted lines and compose the alert body from those localized pieces. Based on learnings: prefer.one/.otherlocalization keys for pluralized Swift UI strings.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/AppDelegate.swift` around lines 5917 - 5929, The alert.informativeText currently uses one localized paragraph with embedded counts, so pluralization can't vary per count; instead create separate plural-aware localized strings for each counted line (use String(localized: "update.relaunchWarning.workspaces", table:nil, count: summary.workspaceCount) with .one/.other keys, similarly for terminalSessionCount, runningCommandCount, and remoteTerminalSessionCount), then build alert.informativeText by concatenating the static header and these four localized lines (referencing alert.informativeText, summary.workspaceCount, summary.terminalSessionCount, summary.runningCommandCount, summary.remoteTerminalSessionCount) so each count uses proper singular/plural localization.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@Sources/AppDelegate.swift`:
- Around line 6025-6033: The scan over window contexts currently only appends
the current tabManager when no managers were found, which can miss live
terminals if mainWindowContexts is stale; modify the logic that builds managers
(the loop over mainWindowContexts and the conditional that appends tabManager)
so you always visit both sources—iterate mainWindowContexts.values and also
visit the current tabManager (if non-nil) and use ObjectIdentifier +
seenManagers.insert(...) to dedupe exactly as is done for the contexts (mirror
forEachTerminalPanel(_:)'s approach), ensuring tabManager is added when not
already seen rather than only when managers.isEmpty.
- Around line 5917-5929: The alert.informativeText currently uses one localized
paragraph with embedded counts, so pluralization can't vary per count; instead
create separate plural-aware localized strings for each counted line (use
String(localized: "update.relaunchWarning.workspaces", table:nil, count:
summary.workspaceCount) with .one/.other keys, similarly for
terminalSessionCount, runningCommandCount, and remoteTerminalSessionCount), then
build alert.informativeText by concatenating the static header and these four
localized lines (referencing alert.informativeText, summary.workspaceCount,
summary.terminalSessionCount, summary.runningCommandCount,
summary.remoteTerminalSessionCount) so each count uses proper singular/plural
localization.
There was a problem hiding this comment.
🧹 Nitpick comments (3)
Sources/Update/UpdateDriver.swift (1)
488-508: Consider making the helperinternalvisibility explicit.Both
makeGuardedUpdateInstallChoiceReplyandmakeGuardedUpdateRelaunchActionlack access modifiers, defaulting tointernal. This is correct for testability (the test file uses them), but adding explicitinternalwould document the intent.📝 Suggested clarity improvement
- func makeGuardedUpdateInstallChoiceReply( + internal func makeGuardedUpdateInstallChoiceReply( _ reply: `@escaping` `@Sendable` (SPUUserUpdateChoice) -> Void ) -> `@Sendable` (SPUUserUpdateChoice) -> Void {- func makeGuardedUpdateRelaunchAction( + internal func makeGuardedUpdateRelaunchAction( applicationTerminated: Bool, action: `@escaping` () -> Void ) -> () -> Void {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/Update/UpdateDriver.swift` around lines 488 - 508, Add the explicit internal access modifier to the helper functions to document intended test visibility: change the declarations of makeGuardedUpdateInstallChoiceReply(_:) and makeGuardedUpdateRelaunchAction(_:) to start with "internal func" (preserving their existing signatures, attributes like `@Sendable`, and capture semantics) so their visibility is explicit for readers and tests.cmuxTests/UpdatePillReleaseVisibilityTests.swift (2)
302-310: Mirror-based introspection is fragile but acceptable for tests.This approach accesses a private property via reflection. If the property name changes in
UpdateDriver, this helper will silently returnnil. Consider adding a comment noting this coupling, or alternatively exposing a test-only accessor.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmuxTests/UpdatePillReleaseVisibilityTests.swift` around lines 302 - 310, The test helper activePresentation uses mirror-based reflection to access UpdateDriver's private property "activeUserInitiatedCheckPresentation", which is fragile if that property is renamed; either add a clear comment above the activePresentation(on:) helper noting this coupling and the risk (mentioning UpdateDriver and the "activeUserInitiatedCheckPresentation" field), or preferably add a test-only accessor on UpdateDriver (e.g., internal or `@testable` var activeUserInitiatedCheckPresentation) and update activePresentation(on:) to use that accessor instead of Mirror so the test is robust to refactors.
235-270: Consider adding a test forapplicationTerminated: truescenario.The current tests cover
applicationTerminated: false, butmakeGuardedUpdateRelaunchActionhas a different code path whenapplicationTerminated: true(it skips confirmation and runs the action immediately). Adding a test would ensure that path is covered.🧪 Suggested additional test
func testGuardedRelaunchActionRunsImmediatelyWhenApplicationTerminated() async { var confirmationCallCount = 0 let driver = makeDriver { confirmationCallCount += 1 return false // Would block if checked } let actionExpectation = expectation(description: "relaunch action invoked") let guardedAction = driver.makeGuardedUpdateRelaunchAction(applicationTerminated: true) { actionExpectation.fulfill() } guardedAction() await fulfillment(of: [actionExpectation], timeout: 1.0) XCTAssertEqual(confirmationCallCount, 0, "Confirmation should not be checked when app is already terminated") }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmuxTests/UpdatePillReleaseVisibilityTests.swift` around lines 235 - 270, Add a new async unit test for the applicationTerminated: true branch of makeGuardedUpdateRelaunchAction that verifies the action runs immediately and the confirmation callback is not invoked: create a driver whose confirmation closure increments a counter and returns false, create an expectation that the relaunch action is invoked, call driver.makeGuardedUpdateRelaunchAction(applicationTerminated: true) { fulfill expectation }, invoke the returned guardedAction(), await the expectation (timeout ~1s) and assert confirmationCallCount == 0 to ensure confirmation was skipped.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@cmuxTests/UpdatePillReleaseVisibilityTests.swift`:
- Around line 302-310: The test helper activePresentation uses mirror-based
reflection to access UpdateDriver's private property
"activeUserInitiatedCheckPresentation", which is fragile if that property is
renamed; either add a clear comment above the activePresentation(on:) helper
noting this coupling and the risk (mentioning UpdateDriver and the
"activeUserInitiatedCheckPresentation" field), or preferably add a test-only
accessor on UpdateDriver (e.g., internal or `@testable` var
activeUserInitiatedCheckPresentation) and update activePresentation(on:) to use
that accessor instead of Mirror so the test is robust to refactors.
- Around line 235-270: Add a new async unit test for the applicationTerminated:
true branch of makeGuardedUpdateRelaunchAction that verifies the action runs
immediately and the confirmation callback is not invoked: create a driver whose
confirmation closure increments a counter and returns false, create an
expectation that the relaunch action is invoked, call
driver.makeGuardedUpdateRelaunchAction(applicationTerminated: true) { fulfill
expectation }, invoke the returned guardedAction(), await the expectation
(timeout ~1s) and assert confirmationCallCount == 0 to ensure confirmation was
skipped.
In `@Sources/Update/UpdateDriver.swift`:
- Around line 488-508: Add the explicit internal access modifier to the helper
functions to document intended test visibility: change the declarations of
makeGuardedUpdateInstallChoiceReply(_:) and makeGuardedUpdateRelaunchAction(_:)
to start with "internal func" (preserving their existing signatures, attributes
like `@Sendable`, and capture semantics) so their visibility is explicit for
readers and tests.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7bdebc0f-fa38-4ec8-87b9-d9e4392f5294
📒 Files selected for processing (3)
Sources/Update/UpdateDelegate.swiftSources/Update/UpdateDriver.swiftcmuxTests/UpdatePillReleaseVisibilityTests.swift
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Notes
Verification
xcodebuild -project GhosttyTabs.xcodeproj -scheme cmux-unit -configuration Debug -destination 'platform=macOS' -derivedDataPath /tmp/cmux-issue-1984-session-persist-update-unit build./scripts/reload.sh --tag issue-1984-session-persist-updateSummary by CodeRabbit
New Features
Localization
Tests